fix(react): onCreate fired twice under React.StrictMode in useEditor - #8095
Open
a-y-ibrahim wants to merge 7 commits into
Open
fix(react): onCreate fired twice under React.StrictMode in useEditor#8095a-y-ibrahim wants to merge 7 commits into
a-y-ibrahim wants to merge 7 commits into
Conversation
React 18/19 StrictMode invokes useEditor's internal useState lazy initializer twice for a single logical mount, keeping only the first call's result. Both calls construct (and auto-mount, since Editor defaults to a detached div) a real Editor, so both schedule their own onCreate via a 0ms timer - the existing scheduleDestroy cleanup runs 1ms later, always losing that race. Track, per useEditor() call, the most recent EditorInstanceManager still unconfirmed as mounted. A duplicate construction now destroys itself synchronously, before its Editor's onCreate has a chance to fire, instead of relying on the slower timer-based race. Fixes ueberdosis#7091
🦋 Changeset detectedLatest commit: 08906c6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 72 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for tiptap-embed ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesReact StrictMode lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Sequence Diagram(s)sequenceDiagram
participant React
participant EditorInstanceManager
participant Editor
React->>EditorInstanceManager: initialize useEditor
Editor->>EditorInstanceManager: emit create
EditorInstanceManager->>EditorInstanceManager: buffer create event
React->>EditorInstanceManager: run mount effect
EditorInstanceManager-->>React: forward onCreate once
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
coderabbitai
Bot
added
area: react
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react/src/useEditor.spec.ts`:
- Around line 196-230: Update the test around TestComponent and render to
capture the returned unmount function, ensure only the discarded StrictMode
duplicate invokes the throwing onDestroy, then call unmount and await
flushTimers before process.off('uncaughtException'). Keep the assertions
verifying rendering does not throw synchronously and the deferred error is
surfaced.
In `@packages/react/src/useEditor.ts`:
- Around line 98-110: Update the cleanup logic around Editor.destroy() to retain
a local reference to the discarded editor and, when destroy throws, attempt that
editor’s unmount() before asynchronously rethrowing the original callback error.
Preserve clearing this.editor in all cases, and add a test verifying a discarded
editor is fully unmounted, including its view and CSS cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 340bbd64-dc57-4aff-bb5e-add9b8c0c502
📒 Files selected for processing (3)
.changeset/fix-react-strictmode-double-oncreate.mdpackages/react/src/useEditor.spec.tspackages/react/src/useEditor.ts
Comment on lines
+98
to
+110
| try { | ||
| this.editor?.destroy() | ||
| } catch (error) { | ||
| // A user callback (e.g. onDestroy) threw. Don't let that propagate | ||
| // through React's render phase - surface it asynchronously instead, | ||
| // same as it would have surfaced before this instance was destroyed | ||
| // synchronously (e.g. via the scheduleDestroy timer below). | ||
| setTimeout(() => { | ||
| throw error | ||
| }) | ||
| } finally { | ||
| this.editor = null | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Finish cleanup when onDestroy throws.
Editor.destroy() emits destroy before calling unmount(). If onDestroy throws, this catch clears the manager reference without unmounting the discarded editor, potentially leaving its view and CSS behind.
Please keep a local editor reference and attempt unmount() in the error path before rethrowing the callback error asynchronously. Add a test that verifies the discarded editor is fully unmounted.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/react/src/useEditor.ts` around lines 98 - 110, Update the cleanup
logic around Editor.destroy() to retain a local reference to the discarded
editor and, when destroy throws, attempt that editor’s unmount() before
asynchronously rethrowing the original callback error. Preserve clearing
this.editor in all cases, and add a test verifying a discarded editor is fully
unmounted, including its view and CSS cleanup.
1 task
The previous commit let both StrictMode-duplicated EditorInstanceManager constructions happen and then synchronously evicted the discarded one. That still let onBeforeCreate/onMount double-fire (unavoidable once a second Editor is constructed at all), and needed extra handling for a user callback throwing during the synchronous eviction. Replace the useState lazy initializer with a useRef guard instead: a ref's `current` is not reset between StrictMode's double invocation of the component function, so checking `if (ref.current === null)` before constructing prevents the second Editor from ever being constructed in the first place. This is simpler, removes the need for the eviction machinery entirely, and also fixes the onBeforeCreate/onMount double-fire as a side effect.
a-y-ibrahim
changed the title
All existing StrictMode-focused tests wrapped everything in React.StrictMode; add a plain-render counterpart so a future change can't silently break normal (non-StrictMode) mounting.
Both the previous commit's useRef guard and PR ueberdosis#8090's identical approach relied on a useRef's `current` persisting across StrictMode's double-invoked lazy initializer call. That assumption does NOT hold on React 18 (verified against 18.2.0 and 18.3.1): each of the two calls gets its own independent ref, so the guard never detects the duplicate, and onCreate still fires twice - the original bug, still present on React 18, which @tiptap/react explicitly supports as a peer dependency. Replace it with a fix that doesn't depend on any cross-invocation persistence at all: buffer the Editor's 'create' event and only forward it to the user's onCreate once the owning EditorInstanceManager is confirmed mounted (its onRender effect actually running). An instance whose effect never runs - the StrictMode duplicate, on any React version - simply never forwards its buffered event. Verified against real React 18.2.0, 18.3.1, and 19.1.0 builds (not just this repo's own React 19 devDependency) using the actual built @tiptap/react dist.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react/src/useEditor.ts`:
- Around line 247-251: Update the pendingOnCreateEvent flush in onRender to
verify the editor has not been destroyed before invoking
options.current.onCreate. Preserve clearing pendingOnCreateEvent, and ensure
destroyed editors do not emit the stale onCreate event.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: f6af352b-6bea-49b3-adf3-a6f233dff8a8
📒 Files selected for processing (3)
.changeset/fix-react-strictmode-double-oncreate.mdpackages/react/src/useEditor.spec.tspackages/react/src/useEditor.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/fix-react-strictmode-double-oncreate.md
- packages/react/src/useEditor.spec.ts
Comment on lines
+247
to
+251
| if (this.pendingOnCreateEvent) { | ||
| const event = this.pendingOnCreateEvent | ||
| this.pendingOnCreateEvent = null | ||
| this.options.current.onCreate?.(event) | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard against flushing onCreate for a destroyed editor.
If scheduleDestroy's 1ms timeout fires before onRender runs (unlikely but possible when the event loop is congested), the editor is destroyed while pendingOnCreateEvent still holds a stale reference. The subsequent onRender would then call onCreate with a destroyed editor, which is confusing for users.
Adding a destroyed check is a one-line defensive guard:
🛡️ Proposed fix
if (this.pendingOnCreateEvent) {
const event = this.pendingOnCreateEvent
this.pendingOnCreateEvent = null
- this.options.current.onCreate?.(event)
+ if (!event.editor.isDestroyed) {
+ this.options.current.onCreate?.(event)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (this.pendingOnCreateEvent) { | |
| const event = this.pendingOnCreateEvent | |
| this.pendingOnCreateEvent = null | |
| this.options.current.onCreate?.(event) | |
| } | |
| if (this.pendingOnCreateEvent) { | |
| const event = this.pendingOnCreateEvent | |
| this.pendingOnCreateEvent = null | |
| if (!event.editor.isDestroyed) { | |
| this.options.current.onCreate?.(event) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/react/src/useEditor.ts` around lines 247 - 251, Update the
pendingOnCreateEvent flush in onRender to verify the editor has not been
destroyed before invoking options.current.onCreate. Preserve clearing
pendingOnCreateEvent, and ensure destroyed editors do not emit the stale
onCreate event.
Confirms onCreate's buffer-until-confirmed mechanism doesn't introduce a new failure mode when a user's onCreate callback throws: it can now fire either from the raw 0ms timer or from within onRender's effect, but neither path lets the exception propagate synchronously through render, matching pre-fix behavior for a throwing onCreate.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react/src/useEditor.spec.ts`:
- Around line 266-267: Update the assertions in the useEditor test to verify
exactly one caught error, and assert that its message matches the expected
onCreate error. Replace the broad Error-instance and count checks around
caughtAsync with deterministic validation of the single delivered error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 149994f9-22e5-43ef-b8ff-5d044782387a
📒 Files selected for processing (1)
packages/react/src/useEditor.spec.ts
Contributor
|
I'm of the opinion that onCreate running twice is not a bug - it should be the default behavior, because it helps enforce React best practices and it aligns with its philosophy of keeping initializers free from side-effects. See my comment here: #7091 (comment) @bdbch let me know if you agree on that. Perhaps I'm wrong. |
Per CodeRabbit review: the fix guarantees onCreate fires exactly once, so the throwing-callback test should assert deterministically on a single caught error and its message, not a loose "at least one, some Error" check.
Verifies the fix holds for the refreshEditorInstance path too, not just the initial mount: onCreate still fires exactly once per deps change under StrictMode (1 for the first mount, then 1 more per subsequent deps change), with no double-firing at any point.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes
Fixes #7091
Changes and Review
useEditor()'sEditorInstanceManageris created via auseStatelazy initializer. React 18/19 StrictMode invokes that initializer twice for a single mount, and both calls construct (and auto-mount, sinceEditordefaults to a detached div) a realEditor, so both used to schedule their owncreateevent.onCreateis now buffered until the owningEditorInstanceManageris confirmed mounted (i.e. its render effect actually runs), and only forwarded to the user's callback then. An instance whose effect never runs - the StrictMode duplicate - simply never forwards its buffered event. This doesn't depend on any cross-invocation persistence (e.g. a ref surviving StrictMode's double-invoke), which turned out not to be reliable across React versions: verified against real React 17.0.2, 18.2.0, 18.3.1, and 19.1.0 builds using the actual built@tiptap/reactdist, not just this repo's own React 19 devDependency.Added
useEditor.spec.ts(@testing-library/react, matchingBubbleMenu.spec.ts's existing pattern) covering: singleonCreateunder StrictMode, independent sibling editors,immediatelyRender: false, a full unmount/remount cycle, and mounting without StrictMode.Checklist
Responsibility